Rewrite config parsing for better span support #13084

Jarcho · 2024-07-11T03:29:15Z

changelog: Improved spans on config errors.
rust-lang/rust-clippy#13084

xFrednet

I really like these changes! One tiny nit, then you can r=me :D

clippy_config/src/conf.rs

xFrednet · 2024-07-13T23:32:58Z

Oh one more thing, do we maybe want to add a lint or warning, if the ".." is not in the last index? I feel like it could be confusing otherwise 🤔

Jarcho · 2024-07-13T23:45:06Z

I'm personally on the side that it should be first since this is extending an existing list. An ellipsis comes first when writing so the thing that's acting like one should also come first.

As for linting when it appears in the middle it might be worth it since it's otherwise silently ignored now.

xFrednet · 2024-07-14T09:02:15Z

I'm personally on the side that it should be first since this is extending an existing list. An ellipsis comes first when writing so the thing that's acting like one should also come first.

I think for a single value, the other thing looks a bit better as in

x = ["custom", ".."]

But with multiple values and potentially multiple lines, it's definitely better to have it in the front. So, that should be our default suggestion.

It's probably easier to make this a warning, and not a lint, since we can then just emit it while reading the config file. It'd be good if this warning/lint could be part of this PR, but we could make it a followup PR, if you prefer.

bors · 2024-07-17T19:04:54Z

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.

Jarcho · 2024-07-18T07:15:28Z

Added a change to the metadata collector. Should this be changed back to printing [] for the default value of an empty array? msrv and trivial_copy_size_limit already didn't list a default value.

For the warning that requires changing how values are deserialized in order to get the span correct. I'll look into it.

xFrednet · 2024-07-18T07:22:27Z

Hmm, I like having the default value for everything for consistency. For booleans etc we also display the default, even if it's the default of those values. So, I'd be in favor of keeping it.

I like that the original order of the values in the default configuration is now retained.

Jarcho · 2024-07-27T20:20:36Z

Latest change is basically a full rewrite using toml_edit instead or serde and toml. This lets us deserialize values with spans and get better error reporting. A bunch of misc. changes are included as well at the moment.

I'll be splitting this up later into multiple commits for easier reading, but you can get a feel for the end result with this. The diff is going to be totally useless right now.

Misc changes to `clippy_config` Contains part of #13084 Changes include: * Sort config list and each configs lint list. * Add default text for the two configs that were missing it. * Switch the lint list in the configs to an attribute. * Make `dev fmt` sort the config list. r? `@xFrednet` changelog: none

xFrednet · 2024-07-30T09:21:19Z

Could you rebase to include the changes from the last PR?

book/src/lint_configuration.md

clippy_config/src/conf.rs

xFrednet

Tiny NIT, the rest looks good to me. We can merge this, once the commit has a proper name and the nit is fixed.

This PR was initially created to restrict the ".." value to the beginning or end. My understanding is that currently all positions are accepted again, right? Adding this restriction and a respective warning can be done in a followup, this PR is already quite large.

xFrednet · 2024-08-01T08:35:49Z

clippy_config/src/de.rs

+macro_rules! deserialize_table {
+    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {


NIT: I think the usage if this macro is not self-explanatory. I would suggest adding a doc comment and renaming it to deserialize_table_row since it only processes one row.

Maybe:

Suggested change

macro_rules! deserialize_table {

($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

/// Can be used to deserialize a table row. Each column has a local

/// of type `Option<T>` that is filled, when the value is found.

///

/// Example usage:

/// ```

/// deserialize_table_row!(dcx, table,

/// path("path"): String,

/// reason("reason"): String,

/// );

/// ```

///

/// Example input:

/// ```toml

/// disallowed-methods = [

/// { path = "std::vec::Vec::leak", reason = "no leaking memory" },

/// # path = Some("std::vec::Vec::leak")

/// # reason = Some("no leaking memory")

///

/// { path = "std::time::Instant::now" },

/// # path = Some("std::vec::Vec::leak")

/// # reason = None

/// ]

/// ```

macro_rules! deserialize_table_row {

($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

What's this about rows? It can deserialize any toml table, both inline and regular. The TableLike trait is implemented for both.

Huh, I guess it depends on what you describe as a "table". I thought the whole disallowed-methods is a table, like this

path reason

"std::vec::Vec::leak" "std::vec::Vec::leak"

"std::time::Instant::now"

But I'm guessing now that you called { path = "std::vec::Vec::leak", reason = "no leaking memory" } a table?

Ah, that's what you mean. The name is referring to a toml table.

Okay, then we can keep it like this. Could you rename the commit and fix the last error? Then we can merge this :D

xFrednet · 2024-08-02T08:35:38Z

Have you seen my question from the last review:

This PR was initially created to restrict the ".." value to the beginning or end. My understanding is that currently all positions are accepted again right? Adding this restriction and a respective warning can be done in a followup, this PR is already quite large.

bors · 2024-08-06T15:13:11Z

☔ The latest upstream changes (presumably #13225) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2025-03-16T14:44:21Z

I'm unassigning myself from this PR. @Jarcho if you want to continue this work, please request a new reviewer with r? clippy

Jarcho · 2025-04-12T20:55:37Z

r? clippy

Jarcho · 2025-04-12T21:26:45Z

disallow_span_lint is removed because the path to clippy.toml isn't normalized (it's in a parent directory of the test). It's a pretty low value test that only checks that clippy has it's own config file set correctly so away it goes.

The part has been split from this PR.

Failing run

Jarcho · 2025-04-21T01:55:03Z

@rustbot ready

Jarcho · 2025-07-18T00:05:37Z

r? clippy

rustbot · 2025-07-21T23:08:35Z

Some changes occurred in clippy_lints/src/doc

cc @notriddle

Jarcho · 2025-07-22T14:16:37Z

I have a plan to remove the double config name foo_bar("foo-bar"), but that will require some changes to clippy_dev. In the meantime cargo dev fmt will error if the two names differ so this is only a minor annoyance. The diagnostic improvements are definitely worth merging despite this.

This will likely be switching to toml instead of toml_edit in the future, but that needs to wait on some api changes for that to be viable.

* More precise spans in diagnostics * Allow multiple diagnostics for each config value * Ability to capture spans in the parsed config * Easier support of `..` in lists * Remove `termize` dependency. Use values from rustc instead.

rustbot · 2025-10-02T14:43:42Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Jarcho · 2025-10-02T14:49:30Z

Switched to the new toml crate. The rest of the repo should probably be moved to this version as well, but that can be a different PR.

With this and #13084 our toml version will be pushed to `0.9.7`. This is different from rustc's version, but the changes in #13084 already require an incompatible version. The `serde` feature is also removed since it was only used in one spot with a mostly trivial replacement. changelog: none

rustbot · 2025-10-07T07:00:22Z

☔ The latest upstream changes (possibly f3c020c) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned xFrednet Jul 11, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 11, 2024

xFrednet approved these changes Jul 13, 2024

View reviewed changes

clippy_config/src/conf.rs Outdated Show resolved Hide resolved

Jarcho mentioned this pull request Jul 17, 2024

Create lint passes using Conf #13088

Merged

Jarcho force-pushed the conf_refactor branch from beda539 to f53fce5 Compare July 17, 2024 22:31

Jarcho mentioned this pull request Jul 28, 2024

Misc changes to clippy_config #13173

Merged

Jarcho force-pushed the conf_refactor branch from e71254c to 9979380 Compare July 30, 2024 12:26

xFrednet reviewed Jul 31, 2024

View reviewed changes

book/src/lint_configuration.md Show resolved Hide resolved

clippy_config/src/conf.rs Outdated Show resolved Hide resolved

xFrednet approved these changes Aug 1, 2024

View reviewed changes

xFrednet mentioned this pull request Aug 12, 2024

Replace the metadata collector with tests #13221

Merged

xFrednet removed their assignment Mar 16, 2025

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2025

Jarcho force-pushed the conf_refactor branch 3 times, most recently from 7cf2738 to 219d1ec Compare April 12, 2025 20:54

Jarcho force-pushed the conf_refactor branch from 22cfaaa to f09a875 Compare April 12, 2025 21:44

This comment has been minimized.

Sign in to view

Jarcho force-pushed the conf_refactor branch 4 times, most recently from 141033c to 2bf8a6b Compare April 21, 2025 01:54

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 21, 2025

This comment has been minimized.

Sign in to view

Jarcho mentioned this pull request Apr 21, 2025

Add a note for disallowed items referring to the config file #14663

Open

Jarcho force-pushed the conf_refactor branch from 2bf8a6b to 76ddb0a Compare April 21, 2025 15:34

This comment has been minimized.

Sign in to view

rustbot assigned flip1995 and unassigned dswij Jul 18, 2025

Jarcho force-pushed the conf_refactor branch from 76ddb0a to 06ad2d3 Compare July 21, 2025 23:08

Jarcho force-pushed the conf_refactor branch 3 times, most recently from 31a3467 to 6d2aa96 Compare July 22, 2025 14:04

Rewrite of the config parsing code

d59a72f

* More precise spans in diagnostics * Allow multiple diagnostics for each config value * Ability to capture spans in the parsed config * Easier support of `..` in lists * Remove `termize` dependency. Use values from rustc instead.

Jarcho force-pushed the conf_refactor branch from 6d2aa96 to d59a72f Compare October 2, 2025 14:43

Jarcho mentioned this pull request Oct 2, 2025

Upgrade toml to 0.9.7 and remove the serde feature #15806

Merged

		macro_rules! deserialize_table {
		($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

-macro_rules! deserialize_table {
-    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {
+/// Can be used to deserialize a table row. Each column has a local
+/// of type `Option<T>` that is filled, when the value is found.
+///
+/// Example usage:
+/// ```
+/// deserialize_table_row!(dcx, table,
+///     path("path"): String,
+///     reason("reason"): String,
+/// );
+/// ```
+///
+/// Example input:
+/// ```toml
+/// disallowed-methods = [
+///     { path = "std::vec::Vec::leak", reason = "no leaking memory" },
+///     # path   = Some("std::vec::Vec::leak")
+///     # reason = Some("no leaking memory")
+///
+///     { path = "std::time::Instant::now" },
+///     # path   = Some("std::vec::Vec::leak")
+///     # reason = None
+/// ]
+/// ```
+macro_rules! deserialize_table_row {
+    ($dcx:ident, $table:ident, $($name:ident($name_str:literal): $ty:ty,)+) => {

path	reason
`"std::vec::Vec::leak"`	`"std::vec::Vec::leak"`
`"std::time::Instant::now"`

Rewrite config parsing for better span support #13084

Are you sure you want to change the base?

Rewrite config parsing for better span support #13084

Uh oh!

Conversation

Jarcho commented Jul 11, 2024 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xFrednet commented Jul 13, 2024

Uh oh!

Jarcho commented Jul 13, 2024

Uh oh!

xFrednet commented Jul 14, 2024

Uh oh!

bors commented Jul 17, 2024

Uh oh!

Jarcho commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Jul 18, 2024

Uh oh!

Jarcho commented Jul 27, 2024

Uh oh!

xFrednet commented Jul 30, 2024

Uh oh!

Uh oh!

Uh oh!

xFrednet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet commented Aug 2, 2024

Uh oh!

bors commented Aug 6, 2024

Uh oh!

xFrednet commented Mar 16, 2025

Uh oh!

This comment has been minimized.

Jarcho commented Apr 12, 2025

Uh oh!

Jarcho commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Jarcho commented Apr 21, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Jarcho commented Jul 18, 2025

Uh oh!

rustbot commented Jul 21, 2025

Uh oh!

Jarcho commented Jul 22, 2025

Uh oh!

rustbot commented Oct 2, 2025

Uh oh!

Jarcho commented Oct 2, 2025

Uh oh!

rustbot commented Oct 7, 2025

Uh oh!

Uh oh!

Jarcho commented Jul 11, 2024 •

edited by rustbot

Loading

Jarcho commented Jul 18, 2024 •

edited

Loading

xFrednet left a comment •

edited

Loading

Jarcho commented Apr 12, 2025 •

edited

Loading